Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert correct Meter name #5403

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Revert correct Meter name #5403

merged 3 commits into from
Sep 6, 2024

Conversation

gitmln
Copy link
Contributor

@gitmln gitmln commented Sep 5, 2024

Fixes #5404

Corrects a bug with wrong meter name that has been changed from Microsoft.Extensions.Diagnostics.ResourceMonitoring to ResourceMonitoring

Microsoft Reviewers: Open in CodeFlow

@gitmln
Copy link
Contributor Author

gitmln commented Sep 5, 2024

Fixes #5404

@gitmln
Copy link
Contributor Author

gitmln commented Sep 5, 2024

@dotnet-policy-service agree

@evgenyfedorov2
Copy link
Contributor

Hi @gitmln,

Thank you for your contribution! Could you please add unit tests for the change made? We would like to avoid repeating this bug in the future.

@@ -155,4 +157,15 @@ public void Provider_Returns_MemoryConsumption()
var usage = WindowsSnapshotProvider.GetMemoryUsageInBytes();
Assert.InRange(usage, 0, long.MaxValue);
}

[Fact]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be [ConditionalFact] to support run on a certain platform only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that all tests for WindowsContainerSnapshotProvider support running on Linux/MacOS. In fact, as I checked a pipeline, they are being executed on Ubuntu without a problem:
image

Question is should they? But I think this out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good observation.
No, that's no correct. The Windows-specific tests should have the following decoration:

[OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX, SkipReason = "Windows specific tests")]

@evgenyfedorov2 evgenyfedorov2 merged commit 858624b into dotnet:main Sep 6, 2024
6 checks passed
@dotnet dotnet deleted a comment from github-actions bot Sep 6, 2024
@dotnet dotnet deleted a comment from github-actions bot Sep 6, 2024
@dotnet dotnet deleted a comment from github-actions bot Sep 6, 2024
@joperezr
Copy link
Member

joperezr commented Sep 6, 2024

/backport to release/8.9

Copy link
Contributor

github-actions bot commented Sep 6, 2024

Started backporting to release/8.9: https://github.com/dotnet/extensions/actions/runs/10741673708

@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.Extensions.Diagnostics.ResourceMonitoring meter name changed to ResourceMonitoring
4 participants